Replace modal windows with floating child windows#32
Conversation
|
This change has an issue:
Looks like this is because before the cahnge the debugger indeeded went away on every step. |
Thanks for testing this. I only tested the windows themselves but not the functionality within the windows. I will take a look. |
|
@desertkun, please take another look. |
|
Did you push updated changes? As of commit 5 days ago, just observed the following
|
|
Yes, there is still a regression, sorry. I will address it later. |
Replace [NSApp runModalForWindow:] in nine utility controllers with addChildWindow: against the main emulator window. Fixes Cmd+C/V (blocked by NSApp modal sessions) and lets utility windows coexist. Design reference: ui/gtk3/gtkui.c:menu_machine_debugger and ui/gtk3/debugger.c:ui_debugger_activate. Removing the modal also removed the implicit synchronization between main and the emulator thread for the Debugger. dispatch_semaphore_t debugger_semaphore replaces it: wait in ui_debugger_activate, signal in deactivate_debugger. Pin the emulation NSTimer to the dedicated emulator thread so it doesn't migrate when unpause is called from main. Emulator.pause and unpause dispatch start/stopEmulationTimer to the emulator thread unconditionally; a `timer != nil` shortcut on the calling thread would be a cross-thread read of an ivar whose only writer is now the emulator thread, and a pause-unpause-pause interleaving where the queued stop has drained but the queued start has not could observe stale nil and skip the second stop. Both timer methods already carry the writer-thread idempotency guard. cocoa_break: matches menu_machine_debugger: if the user has paused via Machine->Pause, release that contribution and set HALTED; otherwise just set HALTED. User-pause is not restored on dismissal. validateMenuItem: grays out Machine->Debugger while a utility holds the pause; opening the Debugger there cannot produce a working Step. The predicate subtracts the user-pause contribution (`> (paused ? 1 : 0)` instead of `> 0 && !paused`) so the disable also fires when Machine->Pause is set on top of a utility window's hold; cocoa_break: would otherwise clear the user-pause but the timer would stay stopped, leaving the HALTED trap unreachable until the user closed the utility. deactivate_debugger leaves the Debugger window visible on Continue and Step, matching GTK (gtk_main_quit doesn't destroy the dialog). handleWillClose: only calls debugger_run when debugger_active==1 (user dismissed via the red X); Continue and Step already ran deactivate, and a second debugger_run would clobber HALTED. The C static deactivate_debugger takes no parameters: the Continue/Break button-state distinction lives only in the ObjC wrapper, and the semaphore signal is unconditional. Add a standard Edit menu (Cut/Copy/Paste/Select All, target=First Responder) to MainMenu.xib in the HIG-correct position between File and View. Necessary because the system-supplied edit handling on text fields was previously masked by the modal session. RollbackController registers a NSWindowWillCloseNotification observer in showWindow:; cancel: now just closes the window and the observer does the unpause + removeChildWindow: teardown. Previously only cancel: ran the teardown, so red-X dismissal leaked the pause counter. PokeFinderController.handleWillClose: now removes its own observer too; the previous code only cleared it in dealloc, so each show/close cycle accumulated a fresh registration and the next close fired handleWillClose once per observer, underflowing fuse_emulation_paused into negative territory. FuseController.mainWindowDidBecomeKey: synthesizes an NSFlagsChanged event carrying the current real modifier flags whenever the main emulator window regains key status, and feeds it to [Emulator instance] flagsChanged:. The emulator's commandDown / shiftDown / optDown / ctrlDown ivars are written only by Emulator.flagsChanged:, which fires only on the key window's responder chain; with the modal gone, releasing Cmd while Preferences was the key window never reached DisplayOpenGLView and commandDown stayed YES, so Emulator.keyDown: dropped every keystroke under `if( NO == commandDown && [characters length] )`. User-visible form: open Settings via Cmd+,, close it, the Spectrum keyboard ignores everything until any modifier press triggers a real NSFlagsChanged on the main window. The releaseCmdKeys: and releaseKey: methods were the modal-era version of the same fix (runModal had already restored the main window as key by the time they posted synthetic NSKeyUp / NSFlagsChanged); without runModal those posts land at the utility window and either no-op or type stray characters into focused fields. Removed along with all 21 call sites now that mainWindowDidBecomeKey: covers the case.
MemoryBrowserController.m and PokeMemoryController.m both declared `id notificationObserver;` at file scope without `static`. The two translation units emit common symbols (nm against the .o files shows `C _notificationObserver` in each); the linker merges them despite GCC_NO_COMMON_BLOCKS=YES on the target, so the controllers share one storage slot for their addObserverForName: tokens. Latent while runModal forbade simultaneous visibility — the slot always held the token of whatever window was currently closing. Reachable now that the windows coexist: open Memory Browser, open Poke Memory (lazy init writes PM's token into the now-MB-occupied slot), close Memory Browser first. MB's block reads the slot, finds PM's token, removes PM's observer. The next Poke Memory close has no observer to fire — unpause never runs and fuse_emulation_paused leaks, freezing the emulator. Adding `static` to both declarations gives each translation unit private storage. nm now reports `d _notificationObserver` in each .o file.
Until this fix the first time the user opened Pokes/Cheats in a session the emulator kept running. Visible symptoms, in order of how likely the user is to notice them: * Memory Browser open, then Pokes/Cheats: closing Pokes/Cheats resumed the emulator with Memory Browser still on screen, and the browser's snapshot started drifting against live memory. * Pokes/Cheats opened alone: z80 kept executing while the user enabled cheats, so a poke could be partially overwritten by the game's own code before it stuck. * Once Pokes/Cheats had been opened and closed once, subsequent utility windows (Memory Browser, Rollback, etc.) stopped pausing the emulator — they opened normally but showed a live, drifting view instead of the expected snapshot. The defect persisted until the next app launch. Root cause: PokeMemory.xib was the only utility xib without visibleAtLaunch="NO" (the attribute defaults to YES when omitted). PokeMemoryController.init touches [self window] to register a close-notification observer, which lazy-loads the xib; with visibleAtLaunch=YES, NSNib orders the window front during the load. By the time showWindow: ran, isVisible was already YES, so its early-return guard returned without calling pause. The close block still called unpause unconditionally, walking the pause counter down by one each cycle. Adding visibleAtLaunch="NO" to PokeMemory.xib lines it up with the other utility xibs. releasedWhenClosed="NO" is added for the same consistency; NSWindowController already forces it at runtime.
JoystickConfigurationController previously did its parent-child teardown in -cancel:, leaving the red-X dismissal path (and the Preferences-close cascade) without a removeChildWindow:. The stale relationship was reconciled on the next showWindow: by the parent check, so the bug was invisible in practice — but the asymmetry diverged from the close-path matrix in spec 004 which mandates a single teardown hook for every closable-with-pause-or-parent panel. Add an NSWindowWillCloseNotification observer in showWindow:; route the unpin through handleWillClose: so every dismissal path (Apply, Cancel, red X, Preferences close cascade) does the same teardown. Joystick is the only utility window whose showWindow: can run a second time without an intervening close: re-clicking Configure for the other joystick reuses the same instance, switching contents via sender's tag. The spec explicitly disallows a singleton early-return guard here. To keep observer registration idempotent across that switch, remove any prior registration before adding.
PreferencesController.handleWillClose: runs read_config_file, machine reselection, scaler hotswap, [machineRomsController setContent:nil], and [machineRoms release] alongside removing its NSWindowWillCloseNotification observer. When [NSApp terminate:] cascade-closes the windows with Preferences still open, the WillClose notification fires the handler and one of those teardowns reaches into NSWindow's notification observer table while it is still being walked. _platform_strcmp segfaults inside __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__. Latent in the runModal era: the modal session blocked Cmd+Q delivery, so the user could not quit with Preferences open. Removing runModal reaches this path. Register a class-target observer for NSApplicationWillTerminateNotification (which fires before the close cascade), set a file-static flag, and short-circuit handleWillClose: when it is set. Settings have already been persisted to NSUserDefaults via the binding chain by the time the user clicks Quit, so the work being skipped is the apply-back path: meaningless at exit and dangerous when invoked from inside the terminate cascade.
|
I implemented the logic similar to GTK: Memory Browser disables the Debugger menu item, but Debugger allows opening Memory Browser. The idea is the following. The Debugger is a control surface (owns the HALTED trap and Step/Continue); the other utility windows are pure viewers of frozen state.
Technically, it should be possible to open utility windows in any order and maintain consistent resulting state, but that would be a bigger rework (not sure if it's justified at this point). |
Problems solved
The above issues do not exist in the GTK version.
What this PR does
Defects exposed by removing runModal
Three latent bugs were unreachable while modal sessions serialized utility-window visibility and blocked Cmd+Q; the architecture change made them reproducible, and they are fixed in this PR:
notificationObserverstorage between Memory Browser and Poke Memory. Both translation units declared the variable at file scope withoutstatic, so the linker merged them into one slot. With the windows now able to coexist, opening MB then PM and closing MB first caused MB's close block to remove PM's observer, leaking the pause counter on PM's close.-cancel:; red-X and the Preferences-close cascade skipped it. Reconciled lazily on the nextshowWindow:before, now handled uniformly via aWillCloseobserver.PreferencesController.handleWillClose:does substantial work (read_config_file, machine reselection, scaler hotswap, ROM controller teardown) that reaches into NSWindow's observer table while it is still being walked by the terminate cascade. Now short-circuited via anNSApplicationWillTerminateNotificationflag; the apply-back path is meaningless at exit.One independent defect was also fixed in passing:
PokeMemory.xibwas missingvisibleAtLaunch="NO", so the lazy xib load that happens during init ordered the window front beforeshowWindow:ran; the early-return guard inshowWindow:then skipped the pause, while the close block still unpaused. Each cycle walked the counter down by one, eventually breaking pause for every utility window until the next app launch.Tests
Manual, AppleScript-driven against an installed build: